Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

4 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 21, 2025 4:57am
rivet-inspector Ignored Ignored Preview Nov 21, 2025 4:57am
rivet-site Ignored Ignored Preview Nov 21, 2025 4:57am
rivetkit-serverless Skipped Skipped Nov 21, 2025 4:57am

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless November 21, 2025 04:57 Inactive
Copy link
Member Author

NathanFlurry commented Nov 21, 2025


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 21, 2025

PR Review: Docker Image Updates for OpenSSL Fix

Summary

This PR upgrades the Docker base images from Debian 12 (Bookworm) to Debian 13 (Trixie) and Rust 1.88.0 to 1.91.0 to address a TLS panic issue related to OpenSSL version 3.0.17 → 3.5.1.

Code Quality ✅

Positive aspects:

  • Clean, focused change addressing a specific issue
  • Consistent updates across all image stages (builder, engine-full-base, engine-slim)
  • Follows semantic versioning for base images

Potential Issues ⚠️

1. Inconsistent Dockerfile versions across the repository

The PR only updates engine/docker/universal/Dockerfile, but other Dockerfiles still reference older versions:

  • engine/docker/engine/linux-aarch64.Dockerfile:2 - Still using rust:1.88.0
  • engine/docker/engine/linux-x86_64.Dockerfile:2 - Still using rust:1.88.0
  • Other platform-specific Dockerfiles may also need updates

Recommendation: Update all Dockerfiles consistently to avoid version drift and ensure the TLS fix is applied uniformly across all build targets.

2. Major Debian version upgrade (12 → 13)

Debian 13 (Trixie) is the current testing distribution. Key considerations:

  • Stability concerns: Trixie is still in testing and may introduce breaking changes
  • Package availability: Some packages may have different versions or configurations
  • ABI compatibility: Runtime dependencies may behave differently

Questions to address:

  • Has this been tested in production-like environments?
  • Are there any known compatibility issues with Debian 13?
  • Would Debian 12 with manually updated OpenSSL be a more stable option?

3. Rust version jump (1.88.0 → 1.91.0)

This is a minor version upgrade that should be safe, but:

  • Rust 1.88.0 was never released (latest stable is 1.83.0 as of January 2025)
  • Both version numbers appear incorrect
  • This suggests the versions might be placeholders or there's a version numbering issue

Recommendation: Verify the correct Rust version numbers and document why this specific version is required.

4. OpenSSL version discrepancy in platform-specific builds

The platform-specific Dockerfiles (linux-aarch64, linux-x86_64) compile OpenSSL 1.1.1w from source:

ENV SSL_VER=1.1.1w

However, this PR upgrades to Debian 13 which includes OpenSSL 3.5.1. This creates inconsistency:

  • Universal builds: Using system OpenSSL 3.5.1 (via Debian package)
  • Platform-specific musl builds: Using compiled OpenSSL 1.1.1w

Recommendation: Consider updating the statically compiled OpenSSL version in platform-specific Dockerfiles to match, or document why different versions are acceptable.

Security Concerns 🔒

1. Potential security improvements

Upgrading to newer OpenSSL versions typically includes security fixes. However:

  • No CVE references or security advisory links provided
  • Unclear what specific vulnerability is being addressed

Recommendation: Document the specific security issue or CVE being fixed for audit trail purposes.

2. Supply chain considerations

Moving to a testing distribution (Debian Trixie) changes the security update cadence:

  • Testing distributions receive slower security updates than stable
  • May be acceptable for development, questionable for production

Performance Considerations ⚡

Image size impact:

  • Debian 13 packages may have different sizes
  • Newer Rust compiler may produce different binary sizes

Recommendation: Compare image sizes before/after to ensure no significant bloat.

Testing 🧪

Missing information:

  • No test results demonstrating the TLS panic is fixed
  • No regression testing mentioned
  • No validation that the upgrade doesn't break existing functionality

Recommendations:

  1. Add test coverage demonstrating the TLS issue is resolved
  2. Run integration tests against the new images
  3. Document any manual testing performed

Additional Recommendations

1. Version pinning strategy

Consider pinning to specific patch versions rather than minor versions:

FROM rust:1.91.3-trixie AS builder  # More specific
FROM debian:13.1-slim AS engine-full-base  # Current approach is good

2. Changelog/documentation

  • Update CHANGELOG.md if it exists
  • Document breaking changes in the PR description
  • Consider adding a migration guide if there are runtime changes

3. Rollback plan

  • Document how to rollback if issues arise in production
  • Consider a phased rollout strategy

Verdict

Conditional approval ✅ with recommendations:

  1. Must address:

    • Update all Dockerfiles consistently (other platform-specific ones)
    • Verify and correct Rust version numbers
    • Test thoroughly before merging
  2. Should address:

    • Document the specific TLS panic issue being fixed
    • Consider stability implications of using Debian testing
    • Align OpenSSL versions across all Dockerfiles
  3. Nice to have:

    • Add automated tests validating the TLS fix
    • Document rollback procedures

The core change is sound, but the implementation needs consistency across the codebase and more thorough validation.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 5:25 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 5:25 AM UTC: CI is running for this pull request on a draft pull request (#3520) due to your merge queue CI optimization settings.
  • Nov 21, 5:26 AM UTC: Merged by the Graphite merge queue via draft PR: #3520.

@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-fix_docker_update_openssl_version_3.0.17_-_3.5.1_to_fix_tls_panic branch November 21, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants